Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enable message receipts by default; closes #132 #194

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

spencerlepine
Copy link
Contributor

@spencerlepine spencerlepine commented Sep 28, 2023

Issue #, if available:
#132

Description of changes:
Make enable message receipts by default without calling setGlobalConfig()

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@spencerlepine spencerlepine added 🐛 Bug Something isn't working ⌛️ In Progress Team is currently working on fix labels Sep 28, 2023
@spencerlepine spencerlepine force-pushed the spenlep/issue-132 branch 2 times, most recently from 670ef09 to c92c3ea Compare September 28, 2023 20:52
@spencerlepine spencerlepine marked this pull request as ready for review September 28, 2023 20:56
@spencerlepine spencerlepine requested a review from a team as a code owner September 28, 2023 20:56
@spencerlepine spencerlepine requested review from mliao95 and haomingli2020 and removed request for a team September 28, 2023 20:56
@spencerlepine spencerlepine removed the ⌛️ In Progress Team is currently working on fix label Sep 28, 2023
@spencerlepine spencerlepine changed the title fix: enable message receipts by default fix: enable message receipts by default; closes #132 Sep 28, 2023
if (!shouldSendMessageReceipts) {
GlobalConfig.removeFeatureFlag(FEATURES.MESSAGE_RECEIPTS_ENABLED);
}
GlobalConfig.updateThrottleTime(1000);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this 1000ms throttle time come from?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was just copy/pasted from the internal code. No test coverage so it looks like the behavior won't change it either way

The README and code will default to 5000 [ref]

@spenlep-amzn spenlep-amzn requested review from spenlep-amzn and haomingli2020 and removed request for spenlep-amzn March 4, 2024 22:07
@spenlep-amzn spenlep-amzn self-requested a review March 4, 2024 22:37
@spenlep-amzn spenlep-amzn merged commit 09218f7 into master Mar 5, 2024
3 checks passed
@spenlep-amzn spenlep-amzn deleted the spenlep/issue-132 branch March 5, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants